Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for externally provided 'lifecycle-mapping-metadata.xml' file #3005

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Dec 21, 2023

@rgrunber
Copy link
Contributor

rgrunber commented Jan 4, 2024

@testforstephen as the original filer for the issue that this intends to fix, let us know if this meets those needs.

Comment on lines 111 to 112
IEclipsePreferences prefs = InstanceScope.INSTANCE.getNode(IMavenConstants.PLUGIN_ID);
String oldMavenLifecycleMappings = prefs.get(MavenPreferenceConstants.P_WORKSPACE_MAPPINGS_LOCATION, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to use its getter api getMavenConfiguration().getWorkspaceLifecycleMappingMetadataFile() to retrieve the current lifecycle mapping config? I see you have used its setter api to update the lifecycle mapping config in subsequent operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to update the projects if the maven lifecycle mapping is not set.
m2e lifecycle mapping is <m2e_core_location>/lifecycle-mapping-metadata.xml by default, VS Code lifecycle mapping is null by default
See https://github.com/eclipse-m2e/m2e-core/blob/db42af86c7976a6131daf4cda1a10857e497bf92/org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/preferences/MavenConfigurationImpl.java#L266

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This touches too much the internal implementation of m2e, which is not a good practice from a maintenance standpoint.

To identify if users have changed the lifecycle mapping in jdt.ls, I prefer to store the lifecycle mapping setting within jdt.ls metadata cache. We can then use jdt.ls preference cache to identify any diff. If any modification is detected, then call m2e API to apply it.

Copy link
Contributor Author

@snjeza snjeza Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying...
I have updated the PR.

if (!Objects.equals(newMavenLifecycleMappings, oldMavenLifecycleMappings)) {
try {
getMavenConfiguration().setWorkspaceLifecycleMappingMetadataFile(newMavenLifecycleMappings);
LifecycleMappingFactory.getWorkspaceMetadata(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this line do and why is it needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you should consider adding a comment to clarify the purpose of this code. Without it, other maintainers may find it hard to understand what it does from the method call alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

…file

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@snjeza snjeza merged commit d8643fc into eclipse-jdtls:master Jan 25, 2024
7 checks passed
@rgrunber rgrunber added this to the End January 2024 milestone Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for externally provided "lifecycle-mapping-metadata.xml" file
3 participants